-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JarReader #16
base: dev
Are you sure you want to change the base?
Add JarReader #16
Conversation
@@ -69,6 +70,8 @@ public static MappingFormat detectFormat(Reader reader) throws IOException { | |||
case "MD:": | |||
case "FD:": | |||
return MappingFormat.SRG; | |||
case "PK\u0003": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct when the jar is read as if it's utf-8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This correctly identifies jar files when using something like
MappingReader.read(Path.of("minecraft-1.18.2-client.jar"), mappingTree);
|
||
@Override | ||
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { | ||
buffer = processFile(file, buffer, visitor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In jars, some directories aren't valid package names, like ones with -
, also the META_INF and the version-specific class files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processFile
ignores any files not ending with ".jar" or ".class", is that enough to skip non valid files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't handle say classes in doc-files or multi-release classes in META-INF. Checking and skipping invalid directories may be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all classes be processed regardless of the directory they're in? processClass
only uses the file content, not the file name or directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this isn't determinate for Multi-release jars, also doc files etc should be ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, are there any directories besides "META-INF" and "doc-files" that need to be filtered out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend skipping any class in any folder with - in name, as those are invalid java package names. However, I don't know how you want to deal with nested jars, and you might still consider jars in those directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now skips any classes in folders with names containing "-". Jars are not affected by this but they should probably be skipped if they are in "doc-files" folders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jars in "doc-files" folders are now skipped.
I wanted to do this in a more layered fashion with mapping packaging as its own encompassing facility, not merged with mapping formats |
Could you elaborate on this please so I can see if this is something I might be able to implement? |
JarReader now uses ZipFile which allows it to read zip files that contain extra data at the beginning of the file. This makes it possible to read the old Minecraft windows_server executables. Nested jars are ignored at the moment |
public static void read(Path path, MappingVisitor mappingVisitor) throws IOException { | ||
AnalyzingVisitor analyzingVisitor = new AnalyzingVisitor(mappingVisitor); | ||
|
||
ZipFile zipFile = new ZipFile(path.toFile()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably use a jar file system instead,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it's really convenient to pass a ZipEntry to ClassReader, what's the benefit of a jar file system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For it's more modern, and you can skip invalid directory with its children (like in a DirectoryStream) than having to step into it like when you are iterating with a zip file.
This PR adds support for reading an empty mapping (source only) from a jar file. The result can be written out as Enigma and Tiny v2, Tiny v1 skips everything without a destination so it can't be used.